Skip to content

adding a password reset system for admins#246

Open
vlad-a-c wants to merge 3 commits intoXGDevGroup:mainfrom
vlad-a-c:main
Open

adding a password reset system for admins#246
vlad-a-c wants to merge 3 commits intoXGDevGroup:mainfrom
vlad-a-c:main

Conversation

@vlad-a-c
Copy link
Copy Markdown
Contributor

since we don't have a mandatory email address entered, the only way of changing a password is for an admin to create a temp pass. i'm also thinking of making the user able to change his own password from the options menu.

@zarvox32
Copy link
Copy Markdown
Contributor

zarvox32 commented May 1, 2026

Reviewed and tested locally on the merge ref. All 49 tests in the three modified test files pass, plus 43 tests in test_server_authorization.py, test_server_message_flow.py, and test_server_core_misc.py — no regressions in the auth/menu surface this PR touches.

The auth changes are the most valuable part: reset_password now invalidates in-memory sessions AND revokes refresh tokens at the DB layer, which closes a real gap where a reset password wouldn't actually log the target out everywhere. The new revoke_user_refresh_tokens correctly uses lower(username) = lower(?) matching the rest of the table.

A few non-blocking observations (take or leave):

nit: getattr(self, "_password_min_length", 8) and the matching _password_max_length default in _handle_reset_password_editboxServer.__init__ always sets these, so the fallback is dead code. Registration in core/server.py uses self._password_min_length directly. Minor consistency thing.

nit: reset-user-password-prompt doesn't include the "or press Escape to cancel" hint that decline-reason-prompt has. An admin who selects the wrong user and wants to back out has no clear way to discover that.

future consideration: The editbox protocol has no masked/password input mode, so the temporary password is likely echoed and spoken by the screen reader as the admin types it. Pre-existing limitation, not introduced here, but worth filing as an issue if we ever add a self-service password change flow — it'd be the same problem at a worse blast radius.

future consideration: After a successful reset the admin lands back on the user list, which matches _ban_user's pattern so it's consistent — but for password reset specifically, returning to the admin menu might be more useful since the typical flow is "reset one user, done." Not worth changing in isolation.

The double trust-level check (decorator + explicit target_record.trust_level.value >= TrustLevel.ADMIN.value guard inside _reset_user_password) is good defense in depth.

LGTM.

@zarvox32 zarvox32 linked an issue May 1, 2026 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature request: Add password retrieval system.

2 participants